-
Notifications
You must be signed in to change notification settings - Fork 4.8k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[wasm] Use rsp file for emcc default flags, and a props file instead of txt #52941
Conversation
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
Tagging subscribers to 'arch-wasm': @lewing Issue DetailsWIP!
|
`emcc-default.rsp`: - this replaces `emcc-flags.txt` which was generated at wasm build time as the default set of flags. `Emcc.props`: - this replaces `emcc-version.txt` which was generated at wasm build time, and contained the output of `emcc --version`, eg: `emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 2.0.12 (d0e647bf266caad50943e78c9841e05e9c499a5d)` - Instead of this, we now generate `Emcc.props`, which has: `$(RuntimeEmccVersionRaw)` - full version string (`emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 2.0.12 (d0e647bf266caad50943e78c9841e05e9c499a5d)`) `$(RuntimeEmccVersion)` - parsed version (`2.0.12`) `$(RuntimeEmccVersionHash)` - parsed hash (`d0e647bf266caad50943e78c9841e05e9c499a5d`) - these might be useful for nugets with native libraries for use with wasm, for example - The default options being used in the `Makefile` have been moved to `wasm.proj`, and used to populate the new files - this tries to retain the same flags as are being used currently, on various platforms (eg. for windows the optimization flags differ)
- this is being done mainly to aid readability, and clarity - a new target `WasmBuildNativeOnly` has been added, which should (eventually) be usable before `publish`. - this commit makes the changes for it, but it hasn't been tested in it's final form
Maybe |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doesn't WasmApp.Native.targets need to be added to the pkgproj?
Good catch! thanks, updated. |
I'm a little worried this was green before adding the files. We keep having problems like this. |
I agree. I was thinking of using the nugets with the wasm.build tests, to start with. It won't be a real workload based test though, since the sdk used is older, so probably wouldn't be prudent to depend on that for workload. |
We should strive to find problems as close to the source as practical, that doesn't mean catching everything but it does mean checking for patterns we've seen before. |
Co-authored-by: Larry Ewing <lewing@microsoft.com>
<PlatformManifestFileEntry Include="emcc-flags.txt" IsNative="true" /> | ||
<PlatformManifestFileEntry Include="emcc-version.txt" IsNative="true" /> | ||
<PlatformManifestFileEntry Include="emcc-default.rsp" IsNative="true" /> | ||
<PlatformManifestFileEntry Include="Emcc.props" IsNative="true" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can runtime packs include a props file that will be automatically loaded?
</Target> | ||
|
||
|
||
<Target Name="_SetupEmscripten"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we move the base emscripten detection to the WorkloadManifest.targets where it will always be available (and knows how to require the workload when appropriate)?
<!-- Adding optimization flag at the top, so it gets precedence --> | ||
<_EmccLDFlags Include="$(EmccLinkOptimizationFlag)" /> | ||
<_EmccLDFlags Include="@(_EmccCommonFlags)" /> | ||
<_EmccLDFlags Include="-s TOTAL_MEMORY=536870912" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should the value here be a property?
Suggestions being tracked in #53394 . |
emcc-default.rsp
:emcc-flags.txt
which was generated at wasm build timeas the default set of flags.
Emcc.props
:this replaces
emcc-version.txt
which was generated at wasm buildtime, and contained the output of
emcc --version
, eg:emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 2.0.12 (d0e647bf266caad50943e78c9841e05e9c499a5d)
Instead of this, we now generate
Emcc.props
, which has:$(RuntimeEmccVersionRaw)
- full version string (emcc (Emscripten gcc/clang-like replacement + linker emulating GNU ld) 2.0.12 (d0e647bf266caad50943e78c9841e05e9c499a5d)
)$(RuntimeEmccVersion)
- parsed version (2.0.12
)$(RuntimeEmccVersionHash)
- parsed hash (d0e647bf266caad50943e78c9841e05e9c499a5d
)these might be useful for nugets with native libraries for use with
wasm, for example
Also, extracted
WasmApp.Native.targets
fromWasmApp.targets